Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix DECRBY LLONG_MIN caused an overflow #1581

Merged
merged 5 commits into from
Jul 13, 2023
Merged

Fix DECRBY LLONG_MIN caused an overflow #1581

merged 5 commits into from
Jul 13, 2023

Conversation

enjoy-binbin
Copy link
Member

Note that this may break compatibility since in the past
doing: DECRBY key -9223372036854775808 would succeed
but create an invalid result. And now we will return an
error rejecting the request.

Before:

127.0.0.1:6666> set key 0
OK
127.0.0.1:6666> decrby key -9223372036854775807
(integer) 9223372036854775807
127.0.0.1:6666> get key
"9223372036854775807"

127.0.0.1:6666> set key 0
OK
127.0.0.1:6666> decrby key -9223372036854775808
(integer) -9223372036854775808
127.0.0.1:6666> get key
"-9223372036854775808"

After:

127.0.0.1:6666> decrby key -9223372036854775808
(error) ERR decrement would overflow

@enjoy-binbin
Fix DECRBY LLONG_MIN caused an overflow
dc5140d 
Note that this may break compatibility since in the past
doing: `DECRBY key -9223372036854775808` would succeed
but create an invalid result. And now we will return an
error rejecting the request.

Before:
```
127.0.0.1:6666> set key 0
OK
127.0.0.1:6666> decrby key -9223372036854775807
(integer) 9223372036854775807
127.0.0.1:6666> get key
"9223372036854775807"

127.0.0.1:6666> set key 0
OK
127.0.0.1:6666> decrby key -9223372036854775808
(integer) -9223372036854775808
127.0.0.1:6666> get key
"-9223372036854775808"
```

After:
```
127.0.0.1:6666> decrby key -9223372036854775808
(error) ERR decrement would overflow
```
caipengbo
caipengbo previously approved these changes Jul 12, 2023
View reviewed changes
@git-hulk
Copy link
Member

git-hulk commented Jul 12, 2023 •
edited
Loading

Hi @enjoy-binbin, thanks for taking care of this. Maybe we should fix it in https://github.com/apache/kvrocks/blob/unstable/src/types/redis_string.cc#L320

@tisonkun
Copy link
Member

@enjoy-binbin Out of curiosity, do you have a fuzzer to test out Kvrocks? Your recent great fixes seem like a combo rather than random findings :D

@enjoy-binbin
Copy link
Member Author

They were actually random findings.
I may be sensitive to this, doing code review and found them (also based on my familiarity with redis)

@enjoy-binbin
Copy link
Member Author

@git-hulk when doing the -1 * increment_, the overflow is happended, so i can not do the check

    auto s = string_db.IncrBy(args_[1], -1 * increment_, &ret);

@tisonkun
Copy link
Member

They were actually random findings.

Haha, cool!

@git-hulk
Copy link
Member

@git-hulk when doing the -1 * increment_, the overflow is happended, so i can not do the check

    auto s = string_db.IncrBy(args_[1], -1 * increment_, &ret);

Got it, thanks

@git-hulk
Copy link
Member

Thanks all, merging...

@git-hulk git-hulk merged commit 94df83d into apache:unstable Jul 13, 2023
@enjoy-binbin enjoy-binbin deleted the decrby_overflow branch July 13, 2023 06:36
git-hulk pushed a commit to git-hulk/kvrocks that referenced this pull request Jul 30, 2023
@enjoy-binbin @git-hulk
Fix DECRBY LLONG_MIN caused an overflow (apache#1581)
7542496 
Note that this may break compatibility since in the past
doing: `DECRBY key -9223372036854775808` would succeed
but create an invalid result. And now we will return an
error rejecting the request.

Before:
```
127.0.0.1:6666> set key 0
OK
127.0.0.1:6666> decrby key -9223372036854775807
(integer) 9223372036854775807
127.0.0.1:6666> get key
"9223372036854775807"

127.0.0.1:6666> set key 0
OK
127.0.0.1:6666> decrby key -9223372036854775808
(integer) -9223372036854775808
127.0.0.1:6666> get key
"-9223372036854775808"
```

After:
```
127.0.0.1:6666> decrby key -9223372036854775808
(error) ERR decrement would overflow
```
@git-hulk git-hulk mentioned this pull request Jul 31, 2023
Merged
git-hulk pushed a commit that referenced this pull request Aug 1, 2023
@enjoy-binbin @git-hulk
Fix DECRBY LLONG_MIN caused an overflow (#1581)
367a0ee 
Note that this may break compatibility since in the past
doing: `DECRBY key -9223372036854775808` would succeed
but create an invalid result. And now we will return an
error rejecting the request.

Before:
```
127.0.0.1:6666> set key 0
OK
127.0.0.1:6666> decrby key -9223372036854775807
(integer) 9223372036854775807
127.0.0.1:6666> get key
"9223372036854775807"

127.0.0.1:6666> set key 0
OK
127.0.0.1:6666> decrby key -9223372036854775808
(integer) -9223372036854775808
127.0.0.1:6666> get key
"-9223372036854775808"
```

After:
```
127.0.0.1:6666> decrby key -9223372036854775808
(error) ERR decrement would overflow
```
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@caipengbo caipengbo caipengbo approved these changes

@git-hulk git-hulk git-hulk approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@enjoy-binbin @git-hulk @tisonkun @caipengbo